P-chain - Memo field zeroed post Durango #2607

abi87 · 2024-01-13T11:43:55Z

Why this should be merged

Post Durango we're going to enforce the Memo field is zero for any P-chain tx

How this works

Introduced extra checks. No changes to wallets and APIs yet. We'll revisit it post Durango activation

How this was tested

CI

abi87 · 2024-01-13T12:29:45Z

vms/components/avax/base_tx.go

@@ -65,3 +65,20 @@ func (t *BaseTx) Verify(ctx *snow.Context) error {
 		return nil
 	}
 }
+
+func VerifyMemoFieldLength(memo types.JSONByteSlice, isDurangoActive bool) error {


I added a different method rather than extending SyntacticVerify with config and chainTIme. We can revisit this choice down the line, in case other syntactic checks will depend on the specific fork used

StephenButtolph

Can we add some tests that fail verification because of non-empty memo fields?

vms/components/avax/base_tx.go

vms/platformvm/txs/executor/staker_tx_verification.go

vms/platformvm/txs/executor/standard_tx_executor.go

vms/platformvm/txs/unsigned_tx.go

marun

I don't see obvious errors, but I'm also not sure how to distill this much copy-pasta test code to be able to catch mistakes. I would highly recommend using table-driven testing, a set of reusable test data, and shared helkpers for this kind of testing to simplify both review and maintenance.

marun · 2024-01-19T15:56:00Z

vms/platformvm/txs/executor/standard_tx_executor_test.go

@@ -1003,7 +1041,951 @@ func TestStandardTxExecutorDurangoAddValidator(t *testing.T) {
 	require.Equal(endTime, val.EndTime)
 }

+func TestDurangoStandardTxExecutorAddPermissionlessValidatorTx(t *testing.T) {
+	require := require.New(t)
+	env := newEnvironment(t, true /*=postBanff*/, true /*=postCortina*/, true /*=postDurango*/)


(No action required) Maybe use structs where naming of variables is desired (also good for optional parameters):

env := newEnvironment(t, envConfig{ postBanff: true, postCortina: true, postDurango: true, }, )

I kept this aligned with other tests. However booleans are not great. In #2169 I have cleaned it up and replace the booleans with a fork enum. I'd defer to cleanup to that PR

marun · 2024-01-19T15:56:43Z

vms/platformvm/txs/executor/standard_tx_executor_test.go

+	env.ctx.Lock.Lock()
+	defer func() {
+		require.NoError(shutdownEnvironment(env))
+		env.ctx.Lock.Unlock()


Is it ok that a failure to shutdown the environment without error won't release the lock? If not, maybe shutdownEnvironment would be a better place to ensure lock release?

I believe it does not really matter in this case, since this happens at the very end of the test. But this is something that can be fixed for all tests. #2491 by @dhrubabasu fixes this concern I believe.

marun · 2024-01-19T16:02:12Z

vms/platformvm/txs/executor/standard_tx_executor_test.go

-		uint64(endTime.Unix()),
-		nodeID,
+	// Build the AddValidatorTx
+	ins, unstakedOuts, stakedOuts, signers, err := env.utxosHandler.Spend(


(No action required) Maybe extract into a helper?

marun · 2024-01-19T16:04:33Z

vms/platformvm/txs/executor/standard_tx_executor_test.go

+		}},
+		Subnet:                   testSubnet1.TxID,
+		AssetID:                  ids.GenerateTestID(),
+		InitialSupply:            10,


(No action required) Lots of magic numbers here. Even in tests, using descriptive variable names can be helpful to declare your intent (or where that is too much at least defining a struct configured with a bunch of magic numbers with an appropriately descriptive name).

These numbers don't really matter here, they're just enough to pass validation.
The whole point is to showcase validation around Memo field. But to get a fully formed tx I have specified other attributes.
Perhabs I shoud mark these values as dummy...? Or even better we should probably have a single UT per transactions, carriying out all the relevant validations per fork

dhrubabasu · 2024-01-19T17:33:15Z

vms/components/avax/base_tx.go

+
+func VerifyMemoFieldLength(memo types.JSONByteSlice, isDurangoActive bool) error {
+	if !isDurangoActive {
+		return nil //  already validated in SyntacticVerify


Suggested change

return nil // already validated in SyntacticVerify

// SyntacticVerify validates this field post-Durango

return nil

dhrubabasu · 2024-01-19T17:34:21Z

vms/platformvm/txs/executor/staker_tx_verification.go

@@ -728,6 +761,10 @@ func verifyTransferSubnetOwnershipTx(
 		return err
 	}

+	if err := avax.VerifyMemoFieldLength(tx.Memo, true /*isDurangoActive*/); err != nil {


Suggested change

if err := avax.VerifyMemoFieldLength(tx.Memo, true /*isDurangoActive*/); err != nil {

if err := avax.VerifyMemoFieldLength(tx.Memo, true /*=isDurangoActive*/); err != nil {

dhrubabasu · 2024-01-19T17:35:22Z

vms/platformvm/txs/executor/standard_tx_executor.go

@@ -512,6 +550,10 @@ func (e *StandardTxExecutor) BaseTx(tx *txs.BaseTx) error {
 		return err
 	}

+	if err := avax.VerifyMemoFieldLength(tx.Memo, true /*isDurangoActive*/); err != nil {


Suggested change

if err := avax.VerifyMemoFieldLength(tx.Memo, true /*isDurangoActive*/); err != nil {

if err := avax.VerifyMemoFieldLength(tx.Memo, true /*=isDurangoActive*/); err != nil {

dhrubabasu · 2024-01-19T17:35:42Z

vms/platformvm/txs/executor/standard_tx_executor.go

@@ -54,13 +54,20 @@ func (e *StandardTxExecutor) CreateChainTx(tx *txs.CreateChainTx) error {
 		return err
 	}

+	var (
+		timestamp       = e.State.GetTimestamp()


nit: timestamp -> currentTimestamp to match the usage in other funcs

dhrubabasu · 2024-01-19T17:35:47Z

vms/platformvm/txs/executor/standard_tx_executor.go

@@ -98,8 +105,15 @@ func (e *StandardTxExecutor) CreateSubnetTx(tx *txs.CreateSubnetTx) error {
 		return err
 	}

+	var (
+		timestamp       = e.State.GetTimestamp()


nit: timestamp -> currentTimestamp to match the usage in other funcs

vms/components/avax/base_tx.go

Co-authored-by: Dhruba Basu <[email protected]> Signed-off-by: Stephen Buttolph <[email protected]>

forbid memo field in durango

d01b7ee

abi87 requested review from danlaine, dhrubabasu and StephenButtolph as code owners January 13, 2024 11:43

abi87 self-assigned this Jan 13, 2024

nit

3181e9a

abi87 commented Jan 13, 2024

View reviewed changes

abi87 added 3 commits January 13, 2024 13:36

nit

5bbe14c

Merge branch 'dev' into p-chain-no-memo-field-durango

46de238

Merge branch 'dev' into p-chain-no-memo-field-durango

6854972

StephenButtolph added this to the v1.10.19 milestone Jan 16, 2024

abi87 added 2 commits January 17, 2024 03:02

Merge branch 'dev' into p-chain-no-memo-field-durango

80332a3

Merge branch 'dev' into p-chain-no-memo-field-durango

9b5ac0a

StephenButtolph changed the base branch from dev to master January 18, 2024 21:00

StephenButtolph reviewed Jan 18, 2024

View reviewed changes

abi87 added 6 commits January 19, 2024 10:14

nits

c0ce5ad

nits

53e9a44

wip: adding UTs for memo length

b8df184

wip: adding UTs for memo length

1fdf681

nits

1b01c22

added UT

3d33013

abi87 requested a review from StephenButtolph January 19, 2024 13:35

Merge branch 'master' into p-chain-no-memo-field-durango

dcf5591

StephenButtolph approved these changes Jan 19, 2024

View reviewed changes

StephenButtolph added the Durango durango fork label Jan 19, 2024

marun approved these changes Jan 19, 2024

View reviewed changes

dhrubabasu reviewed Jan 19, 2024

View reviewed changes

nits

1e10b39

abi87 requested a review from dhrubabasu January 19, 2024 17:41

dhrubabasu reviewed Jan 19, 2024

View reviewed changes

vms/components/avax/base_tx.go Outdated Show resolved Hide resolved

StephenButtolph and others added 2 commits January 19, 2024 15:19

Update vms/components/avax/base_tx.go

557bfb9

Co-authored-by: Dhruba Basu <[email protected]> Signed-off-by: Stephen Buttolph <[email protected]>

Merge branch 'master' into p-chain-no-memo-field-durango

99e56de

dhrubabasu approved these changes Jan 19, 2024

View reviewed changes

nits (#2640)

6009d83

dhrubabasu approved these changes Jan 19, 2024

View reviewed changes

StephenButtolph enabled auto-merge January 19, 2024 22:20

StephenButtolph added this pull request to the merge queue Jan 19, 2024

Merged via the queue into master with commit 3d4feed Jan 19, 2024
17 checks passed

StephenButtolph deleted the p-chain-no-memo-field-durango branch January 19, 2024 22:47

dhrubabasu mentioned this pull request Feb 13, 2024

Remove Memo field from P-chain wallet #2732

Closed

chand1012 pushed a commit to multisig-labs/avalanchego that referenced this pull request Oct 1, 2024

Remove unused crypto tooling (ava-labs#2607)

05e1dd8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

P-chain - Memo field zeroed post Durango #2607

P-chain - Memo field zeroed post Durango #2607

abi87 commented Jan 13, 2024 •

edited

Loading

abi87 Jan 13, 2024

StephenButtolph left a comment

marun left a comment •

edited

Loading

marun Jan 19, 2024

abi87 Jan 19, 2024

marun Jan 19, 2024

abi87 Jan 19, 2024

marun Jan 19, 2024

marun Jan 19, 2024

abi87 Jan 19, 2024

dhrubabasu Jan 19, 2024

abi87 Jan 19, 2024

dhrubabasu Jan 19, 2024

abi87 Jan 19, 2024

dhrubabasu Jan 19, 2024

abi87 Jan 19, 2024

dhrubabasu Jan 19, 2024

abi87 Jan 19, 2024

dhrubabasu Jan 19, 2024

abi87 Jan 19, 2024

	return nil // already validated in SyntacticVerify
	// SyntacticVerify validates this field post-Durango
	return nil

	if err := avax.VerifyMemoFieldLength(tx.Memo, true /isDurangoActive/); err != nil {
	if err := avax.VerifyMemoFieldLength(tx.Memo, true /=isDurangoActive/); err != nil {

P-chain - Memo field zeroed post Durango #2607

P-chain - Memo field zeroed post Durango #2607

Conversation

abi87 commented Jan 13, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

Choose a reason for hiding this comment

StephenButtolph left a comment

Choose a reason for hiding this comment

marun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abi87 commented Jan 13, 2024 •

edited

Loading

marun left a comment •

edited

Loading